Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: bump all dev config #1301

Merged
merged 16 commits into from
Oct 11, 2021
Merged

deps: bump all dev config #1301

merged 16 commits into from
Oct 11, 2021

Conversation

williaster
Copy link
Collaborator

@williaster williaster commented Aug 2, 2021

🏠 Internal

This PR

  • updates all of our dev config (babel, eslint, typescript, prettier) to the latest that is possible with the nimbus config we currently use
  • updates TS / lint issues throughout all packages
    • the most significant change was adding @visx/scale to @visx/brush because we used made up Scale types in brush which I could no longer get to work

@gazcn007 if this is merged before #1268, there will likely be merge conflicts because of some prettier updates. they shouldn't be too crazy, though. We can try and get yours in before this as well if you'd rather.

@kristw @gazcn007

@github-actions
Copy link

github-actions bot commented Aug 3, 2021

Size Changes

Package Diff ESM Prev ESM CJS Prev CJS
visx-annotation +0.2% 28.35 KB 28.29 KB 39.29 KB 38.11 KB
visx-axis -0.5% 20.37 KB 20.48 KB 24.75 KB 24.78 KB
visx-bounds +6.5% 2.96 KB 2.78 KB 3.37 KB 3.19 KB
visx-brush +2.0% 54.77 KB 53.7 KB 58.82 KB 57.54 KB
visx-chord +0.8% 3.4 KB 3.38 KB 4.61 KB 4.58 KB
visx-clip-path +1.8% 4.4 KB 4.32 KB 5.92 KB 5.84 KB
visx-event +0.3% 3.72 KB 3.71 KB 5.08 KB 5.05 KB
visx-geo +1.3% 13.11 KB 12.95 KB 16.31 KB 16.13 KB
visx-glyph +1.5% 14.75 KB 14.54 KB 19.54 KB 19.33 KB
visx-gradient +1.9% 17.71 KB 17.38 KB 22.32 KB 21.99 KB
visx-grid +0.7% 18.61 KB 18.48 KB 22.39 KB 22.25 KB
visx-group +1.7% 1.61 KB 1.58 KB 2.22 KB 2.19 KB
visx-heatmap +1.8% 7.24 KB 7.12 KB 8.56 KB 8.42 KB
visx-hierarchy -0.5% 11.92 KB 11.98 KB 17.69 KB 17.65 KB
visx-legend +0.3% 26.44 KB 26.37 KB 33.48 KB 33.24 KB
visx-marker +1.6% 8.89 KB 8.75 KB 11.07 KB 10.93 KB
visx-network +3.2% 4.58 KB 4.44 KB 6.71 KB 6.55 KB
visx-pattern +0.4% 11.55 KB 11.5 KB 15.58 KB 15.54 KB
visx-react-spring +0.9% 13.46 KB 13.34 KB 18.34 KB 17.78 KB
visx-responsive +2.6% 21.94 KB 21.39 KB 27.26 KB 26.33 KB
visx-scale +0.3% 18.08 KB 18.02 KB 29.68 KB 29.01 KB
visx-shape +0.2% 84.19 KB 84.04 KB 105.82 KB 104.53 KB
visx-stats -0.6% 13.51 KB 13.58 KB 15.07 KB 15.13 KB
visx-text -0.1% 8.31 KB 8.32 KB 10.98 KB 10.72 KB
visx-tooltip +2.2% 14.13 KB 13.82 KB 20.97 KB 19.93 KB
visx-voronoi +1.2% 2.26 KB 2.23 KB 2.96 KB 2.93 KB
visx-wordcloud +1.4% 2.6 KB 2.56 KB 4.56 KB 4.33 KB
visx-xychart +0.2% 165.95 KB 165.69 KB 238.37 KB 230.88 KB
visx-zoom -0.4% 15.18 KB 15.24 KB 18.18 KB 18.05 KB

Compared to master. File sizes are unminified and ungzipped.

View raw build stats

Previous (master)

{
  "visx-annotation": {
    "esm": 28965,
    "lib": 39026
  },
  "visx-axis": {
    "esm": 20970,
    "lib": 25370
  },
  "visx-bounds": {
    "esm": 2842,
    "lib": 3264
  },
  "visx-brush": {
    "esm": 54984,
    "lib": 58922
  },
  "visx-chord": {
    "esm": 3459,
    "lib": 4688
  },
  "visx-clip-path": {
    "esm": 4421,
    "lib": 5978
  },
  "visx-curve": {
    "esm": 323,
    "lib": 1464
  },
  "visx-demo": {
    "esm": 0,
    "lib": 0
  },
  "visx-drag": {
    "esm": 6751,
    "lib": 8815
  },
  "visx-event": {
    "esm": 3797,
    "lib": 5172
  },
  "visx-geo": {
    "esm": 13259,
    "lib": 16519
  },
  "visx-glyph": {
    "esm": 14893,
    "lib": 19789
  },
  "visx-gradient": {
    "esm": 17800,
    "lib": 22517
  },
  "visx-grid": {
    "esm": 18922,
    "lib": 22784
  },
  "visx-group": {
    "esm": 1619,
    "lib": 2246
  },
  "visx-heatmap": {
    "esm": 7286,
    "lib": 8622
  },
  "visx-hierarchy": {
    "esm": 12266,
    "lib": 18076
  },
  "visx-legend": {
    "esm": 26999,
    "lib": 34033
  },
  "visx-marker": {
    "esm": 8962,
    "lib": 11197
  },
  "visx-mock-data": {
    "esm": 326005,
    "lib": 329416
  },
  "visx-network": {
    "esm": 4546,
    "lib": 6706
  },
  "visx-pattern": {
    "esm": 11779,
    "lib": 15910
  },
  "visx-point": {
    "esm": 819,
    "lib": 1094
  },
  "visx-react-spring": {
    "esm": 13660,
    "lib": 18210
  },
  "visx-responsive": {
    "esm": 21901,
    "lib": 26961
  },
  "visx-scale": {
    "esm": 18452,
    "lib": 29710
  },
  "visx-shape": {
    "esm": 86057,
    "lib": 107037
  },
  "visx-stats": {
    "esm": 13911,
    "lib": 15494
  },
  "visx-text": {
    "esm": 8518,
    "lib": 10981
  },
  "visx-threshold": {
    "esm": 2911,
    "lib": 3820
  },
  "visx-tooltip": {
    "esm": 14147,
    "lib": 20413
  },
  "visx-visx": {
    "esm": 1467,
    "lib": 4243
  },
  "visx-voronoi": {
    "esm": 2286,
    "lib": 3005
  },
  "visx-wordcloud": {
    "esm": 2624,
    "lib": 4431
  },
  "visx-xychart": {
    "esm": 169667,
    "lib": 236421
  },
  "visx-zoom": {
    "esm": 15606,
    "lib": 18482
  }
}

Current

{
  "visx-annotation": {
    "esm": 29032,
    "lib": 40229
  },
  "visx-axis": {
    "esm": 20859,
    "lib": 25340
  },
  "visx-bounds": {
    "esm": 3028,
    "lib": 3450
  },
  "visx-brush": {
    "esm": 56089,
    "lib": 60231
  },
  "visx-chord": {
    "esm": 3486,
    "lib": 4716
  },
  "visx-clip-path": {
    "esm": 4502,
    "lib": 6062
  },
  "visx-curve": {
    "esm": 323,
    "lib": 1464
  },
  "visx-demo": {
    "esm": 0,
    "lib": 0
  },
  "visx-drag": {
    "esm": 6751,
    "lib": 9001
  },
  "visx-event": {
    "esm": 3807,
    "lib": 5200
  },
  "visx-geo": {
    "esm": 13425,
    "lib": 16705
  },
  "visx-glyph": {
    "esm": 15109,
    "lib": 20013
  },
  "visx-gradient": {
    "esm": 18135,
    "lib": 22853
  },
  "visx-grid": {
    "esm": 19057,
    "lib": 22924
  },
  "visx-group": {
    "esm": 1646,
    "lib": 2274
  },
  "visx-heatmap": {
    "esm": 7414,
    "lib": 8770
  },
  "visx-hierarchy": {
    "esm": 12209,
    "lib": 18111
  },
  "visx-legend": {
    "esm": 27079,
    "lib": 34279
  },
  "visx-marker": {
    "esm": 9101,
    "lib": 11337
  },
  "visx-mock-data": {
    "esm": 326005,
    "lib": 329416
  },
  "visx-network": {
    "esm": 4690,
    "lib": 6869
  },
  "visx-pattern": {
    "esm": 11826,
    "lib": 15957
  },
  "visx-point": {
    "esm": 819,
    "lib": 1094
  },
  "visx-react-spring": {
    "esm": 13780,
    "lib": 18784
  },
  "visx-responsive": {
    "esm": 22467,
    "lib": 27919
  },
  "visx-scale": {
    "esm": 18512,
    "lib": 30389
  },
  "visx-shape": {
    "esm": 86210,
    "lib": 108359
  },
  "visx-stats": {
    "esm": 13830,
    "lib": 15432
  },
  "visx-text": {
    "esm": 8510,
    "lib": 11240
  },
  "visx-threshold": {
    "esm": 2911,
    "lib": 3820
  },
  "visx-tooltip": {
    "esm": 14465,
    "lib": 21476
  },
  "visx-visx": {
    "esm": 1467,
    "lib": 4429
  },
  "visx-voronoi": {
    "esm": 2313,
    "lib": 3033
  },
  "visx-wordcloud": {
    "esm": 2661,
    "lib": 4673
  },
  "visx-xychart": {
    "esm": 169936,
    "lib": 244090
  },
  "visx-zoom": {
    "esm": 15541,
    "lib": 18621
  }
}

@williaster williaster marked this pull request as ready for review August 3, 2021 01:17
@williaster williaster changed the title deps: bump all nimbus config deps: bump all dev config Aug 3, 2021
@azemetre azemetre mentioned this pull request Aug 9, 2021
@williaster williaster force-pushed the chris--bump-dev-config branch 2 times, most recently from e5dc2f8 to 21f8b82 Compare September 10, 2021 18:48
@@ -74,9 +74,10 @@
"@testing-library/user-event": "^11.0.1",
"chalk": "4.1.0",
"coveralls": "^3.0.6",
"enzyme": "^3.10.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the ending space is unnecessary.
I will open a PR soon to convert the rest of enzyme tests into RTL. So we can get rid of enzyme from dev dependency : )

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweeeeeeet re enzyme! that will be amazing 🦾

Copy link

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few comments, but stamping to unblock.

one thing i've found useful for these types of PRs in the future is to have 3 different commits added to the PR. The first one is all the config/package.json changes. The second is after running yarn install and autofixing everything. Then the third commit contains all manual changes. That way a reviewer can look at the first and third commits only for review, and ignore the second. food for thought!

@@ -37,7 +37,7 @@ export function getDomainFromExtent(
const invertedEnd = scaleInvert(scale, end + (end < start ? -tolerentDelta : tolerentDelta));
const minValue = Math.min(invertedStart, invertedEnd);
const maxValue = Math.max(invertedStart, invertedEnd);
if (scale.invert) {
if ('invert' in scale && typeof scale.invert !== 'undefined') {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this remain if ('invert' in scale && scale.invert) {? Not sure what types invert can be

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is just the way TS wants to check for whether it's defined. it's either a function or undefined

if (source) ribbon.source(source);
if (target) ribbon.target(target);
if (radius) setNumberOrNumberAccessor(ribbon.radius, radius);
if (startAngle) setNumberOrNumberAccessor(ribbon.startAngle, startAngle);
if (endAngle) setNumberOrNumberAccessor(ribbon.endAngle, endAngle);
const path = (ribbon(chord) as unknown) as string | null;
const path = ribbon(chord) as unknown as string | null;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we still need as unknown?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah 😢

);
};
export default LinkTypesPage;
import React from 'react';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't understand this diff
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, no idea

@@ -17,7 +17,8 @@ describe('sclaeRadial()', () => {

it('set unknown', () => {
const scale = scaleRadial({ domain: [0, 10], unknown: 'green' });
expect(scale('sandwich' as any)).toEqual('green');
// coerce to make TS happy
expect(scale('sandwich' as unknown as number)).toEqual('green');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait, what?

scale seems to only expect a number. you're passing in sandwich and forcing typescript to be ok with it. Shouldn't this test be deleted since TS makes sure this can never happen?

Or, if you want to test the unknown values, can we pass in a number that's not in the domain to test that? either -1 or 11?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think although TS would def catch it, we wanted it to be robust for folks who don't use TS. I'd rather keep this as originally implemented since I only altered the type casting here.

@williaster
Copy link
Collaborator Author

thanks for the review @etr2460 !

one thing i've found useful for these types of PRs in the future is to have 3 different commits added to the PR. The first one is all the config/package.json changes. The second is after running yarn install and autofixing everything. Then the third commit contains all manual changes. That way a reviewer can look at the first and third commits only for review, and ignore the second. food for thought!

💯 I agree this is ideal. I think there was an issue here where auto-fixing actually introduced a lot of any/unknowns which weren't actually what we wanted (like possibly lint fixes broke TS, it's been a while now but something like that)

@williaster williaster merged commit 1ee35ff into master Oct 11, 2021
@williaster williaster deleted the chris--bump-dev-config branch October 11, 2021 19:32
@github-actions
Copy link

🎉 This PR is included in version v2.1.2 of the packages modified 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants